fix(triage): skip code label for closed issues - #1120
Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
PR Summary by QodoSkip ready-to-code labeling for closed or unverifiable issues
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Check the current GitHub issue state immediately before applying the deferred ready-to-code label. Fail closed for that routing label when the state cannot be verified. Refs fullsend-ai/fullsend#6156 Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
b3d9185 to
58cbd92
Compare
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Sanitize the issue number in the workflow warning and verify that API lookup failures preserve informational labels and the triage comment. Refs fullsend-ai/fullsend#6156 Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
Write JSON fixtures with printf and scope mock issue state to each command. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
|
Can you modify this to not be special-cased only for github? |
Yes, good point. on it. Adding Jira and Gitlab as well |
The ready-to-code closed-issue guard was gated on FULLSEND_TRACKER=github, so GitLab and Jira issues could still be routed to the code agent after being closed. Add a tracker_issue_state operation to each tracker ops library (github/gitlab/jira) that normalizes the native state to open/closed and fails non-zero when it cannot be determined, then have post-triage call it tracker-agnostically. The guard continues to fail closed for both a verified-closed issue and an unverifiable one. Add GitLab and Jira regression tests covering the closed-issue skip and the state-lookup-failure skip, and regenerate the bundled scripts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep on head 90bddb4: 1 HIGH, 3 MEDIUM, posted inline.
- HIGH
scripts/post-triage-test.sh:2164— CItestjob is red (shellcheck SC1010, unquoteddone). - MEDIUM
scripts/post-triage.src.sh:731— lookup failure leaves an open bug with no routing label and a green run. - MEDIUM
scripts/lib/jira-triage-ops.lib.sh:345— Jiraundefinedstatus category routed as open, contradicting fail-closed. - MEDIUM
scripts/post-triage.src.sh:726— comment cites an unmerged guard as existing; both PRs carryFixes fullsend#6156.
| jira_closed_exit=0 | ||
| ( | ||
| cd "${jira_closed_dir}" | ||
| MOCK_JIRA_STATUS_CATEGORY=done bash "${POST_SCRIPT}" |
There was a problem hiding this comment.
[HIGH] CI test job is red: shellcheck SC1010 on the new Jira fixture line
MOCK_JIRA_STATUS_CATEGORY=done bash "${POST_SCRIPT}" (added in 90bddb4) trips SC1010 because unquoted done is parsed as the loop keyword. The CI test job on head 90bddb4 fails in the pre-commit shellcheck hook with exactly In scripts/post-triage-test.sh line 2164: ... SC1010, and shellcheck 0.11.0 reproduces it locally on the checked-out head. This undoes commit d48ebc4 ("satisfy shellcheck for state fixtures").
The PR body's Testing section lists make lint, but that Makefile target only runs uvx skillsaw; shellcheck runs only via pre-commit (.pre-commit-config.yaml shellcheck-py hook with -x -e SC1091,SC2001,SC2016), so the listed testing did not cover this.
Suggestion: quote the value:
| MOCK_JIRA_STATUS_CATEGORY=done bash "${POST_SCRIPT}" | |
| MOCK_JIRA_STATUS_CATEGORY="done" bash "${POST_SCRIPT}" |
Run pre-commit run shellcheck --all-files (what CI runs) before pushing, and adjust the Testing section so it does not imply make lint covers shellcheck.
| # workflow (fullsend-ai/fullsend#6876), which reads the triggering event's | ||
| # issue state. | ||
| if ! ISSUE_STATE=$(tracker_issue_state); then | ||
| echo "::warning::Unable to verify issue #$(_gha_sanitize "${ISSUE_NUMBER}") state; skipping ready-to-code label" |
There was a problem hiding this comment.
[MEDIUM] Unverifiable issue state leaves an open bug with no routing label and a green run
When tracker_issue_state fails (transient 5xx/429/network, or the fail-closed *) return 1 branches), this guard emits ::warning:: and sets DEFERRED_LABEL="", so for an open, sufficient bug the run exits 0 having applied bug + the triage comment but neither ready-to-code nor triaged. The issue then has no routing label and nothing re-triggers triage; the only trace is a workflow annotation. The routing branch at lines 448-455 only has the triaged fallback for the WORKFLOW_BLOCKED/auto-code-off paths.
All three tracker_issue_state implementations discard stderr (2>/dev/null), so the warning cannot distinguish 401/403/404/5xx, and there is no retry. The PR body says the routing label is skipped when state cannot be verified, which covers withholding ready-to-code but does not address the absence of any fallback signal.
To be fair, the script's existing convention for read failures is ::warning:: + continue (lines 212, 525, 668), so the soft-fail itself is consistent; the gap is the missing fallback label.
Suggestion: on the lookup-failure branch (as opposed to a confirmed-closed result), fall back to tracker_add_label "triaged" instead of leaving DEFERRED_LABEL empty, mirroring the WORKFLOW_BLOCKED precedent at lines 439-440, so a transient blip visibly parks the issue for human follow-up. Optionally retry the lookup 2-3 times with a short backoff before failing closed, and capture stderr into a sanitized reason in the warning. Regenerate the bundle (make bundle) and add a test asserting triaged is applied on the api-error path for at least one tracker.
| case "${category}" in | ||
| done) printf 'closed\n' ;; | ||
| "") return 1 ;; | ||
| *) printf 'open\n' ;; |
There was a problem hiding this comment.
[MEDIUM] Jira undefined status category is routed as open, contradicting the fail-closed contract
Jira's statusCategory.key has four documented values: undefined ("No Category", id 1), new, indeterminate, done. The new tracker_issue_state returns 1 for an empty key, but this wildcard branch maps undefined (and any future/drifted key, or a case-shifted Done) to open. That is the one case where the tracker itself says it cannot categorise the status, yet the caller applies ready-to-code and dispatches the code agent. This contradicts the function's own doc comment ("Returns non-zero if the state cannot be determined (caller fails closed)") and the PR's stated fail-closed design. The same code is bundled into post-triage.sh:875 and pre-triage.sh:871.
Suggestion: allowlist the known in-flight keys and fail closed on everything else:
case "${category}" in
done) printf 'closed\n' ;;
new | indeterminate) printf 'open\n' ;;
*) return 1 ;;
esacUpdate the doc comment to enumerate the four keys, regenerate the bundles (make bundle / make check-bundle), and add Jira regression cases for new (still open) and undefined (skips ready-to-code).
| # Best-effort producer-side guard: don't route a closed issue to the code | ||
| # agent. tracker_issue_state normalizes each tracker's state to open/closed | ||
| # and returns non-zero when it cannot be determined, so we fail closed for | ||
| # both a verified-closed issue and an unverifiable one. The authoritative |
There was a problem hiding this comment.
[MEDIUM] Both PRs carry Fixes fullsend#6156, and this comment states an unmerged guard already exists
This block comment says the authoritative race guard "lives in the dispatch workflow (fullsend-ai/fullsend#6876)". As of today, fullsend#6876 is still OPEN (not merged) and fullsend#6156 is OPEN, so no such guard exists on either main branch. Both this PR (Fixes fullsend-ai/fullsend#6156) and #6876 (Fixes #6156) claim to close the same issue, so whichever merges first auto-closes #6156 with the other half of the fix still outstanding.
Additionally, consumers pin the floating v0 tag, which currently points at the v0.39.0 base commit c4e059f, so this producer-side guard does not reach fleet runs until the next agents release re-points the tag.
This is adjacent to the closure-race thread above (answered by pointing at #6876), but the point here is the accuracy of the cross-reference and issue-closing hygiene, which that thread does not address. Process-level rather than a code defect.
Suggestion: reword to "the authoritative guard is tracked in fullsend-ai/fullsend#6876" (or merge #6876 first). Keep Fixes on exactly one PR, preferably #6876 since it closes the race, and use Refs fullsend-ai/fullsend#6156 here. Note in the PR body that the change reaches fleet runs only after the next agents release.
3 participants
Summary
ready-to-codelabelThe authoritative dispatch-side guard for the remaining read/write race is in fullsend-ai/fullsend#6876.
Testing
PATH=/opt/homebrew/bin:$PATH make check-bundlePATH=/opt/homebrew/bin:$PATH make lintSCRIPT_TEST_TARGET=source bash scripts/post-triage-test.shSCRIPT_TEST_TARGET=bundled bash scripts/post-triage-test.shThe focused source and bundled suites pass. The broader local
make script-testmatrix reached the gitlint test and stopped becausegitlintis not installed locally; CI runs the dependency-equipped matrix.Fixes fullsend-ai/fullsend#6156